Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for setting function properties using Doxygen #32972

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Mar 5, 2021

Current approach

This PR is an attempt to resolve #21061 by using Doxygen aliases. Developer can add a list of function properties by using @funcprops \prop1, ..., \propN. They will be rendered as plain text in the Doxygen build and a with a link to terminology on the Sphinx side. Note that this solution does not allow to obtain a list of, e.g. "all functions that are async".

Doxygen output:
image

Sphinx output:
image

Pros: Minimal changes, standardized keywords, renders links on the Sphinx side
Cons: Plain-text on the Doxygen side, not possible to list functions with a certain property

Other options

Links to terminology in both Doxygen and Sphinx

In order to achieve this functionality the terminology.rst content needs to be moved to a Doxygen page. Then, the ALIAS can contain, e.g. @ref async. The content of the Doxygen page can be rendered back in Sphinx using the recently added ..doxygenpage directive in breathe, together with breathe-doc/breathe#645.

Doxygen output:
image

image

Sphinx output:

image

image

Pros: Function properties contain link to terminology in both, Doxygen and Sphinx.
Cons: Terminology has to be moved to Doxygen

List of functions with a certain property

In order to obtain a list of functions that have a certain property, e.g. async I have tried the options listed below. Note that I haven't had success in obtaining a fully functional solution.

\xrefitem

Using \xrefitem <key> "(heading)" "(list title)" { text } a new page is created in the Doxygen side with a list of all functions that have the property set. One of the problems is that the generated page is added to the root of the Doxygen tree. Furthermore, I haven't been able to make this option work reliably when multiple properties are used.

Doxygen:

image

Sphinx:

image

image

Pros: Pages with a list of all functions with a certain property are generated
Cons: Not working properly on the Sphinx side (renders empty), page location can't be controlled in the Doxygen side

\ingroup

The advantage of using groups is that pages location can be controlled (e.g. creating a parent group somewhere in the hierarchy). However, the function will be listed in only one group, so it is not an option.

From Doxygen documentation:

Note that compound entities (like classes, files and namespaces) can be put into multiple groups, but members (like variable, functions, typedefs and enums) can only be a member of one group (this restriction is in place to avoid ambiguous linking targets in case a member is not documented in the context of its class, namespace or file, but only visible as part of a group).

Pros: None
Cons: Does not work when multiple properties are used.

Table of properties

In order to generate something like:

function syscall reschedule sleep no-wait isr-ok pre-kernel-ok async supervisor
k_mutex_init YES no no yes yes
k_mutex_lock YES yes YES YES NO*

I haven't found a solution using existing Doxygen/Sphinx/Breathe facilities. On the Doxygen side we should have conditionals to evaluate certain properties of a function. As fas as I know, this is not possible. Onthe Sphinx side we need to implement a new custom plugin that parses Doxygen XML files and generates such table. It may be possible to re-use breathe parser, but I'm not sure it it is worth for this purpose due to its "complexity".

@gmarull gmarull requested a review from carlescufi March 5, 2021 11:17
@github-actions github-actions bot added the area: API Changes to public APIs label Mar 5, 2021
@gmarull gmarull added the DNM This PR should not be merged (Do Not Merge) label Mar 5, 2021
@gmarull gmarull marked this pull request as draft March 5, 2021 11:20
@gmarull gmarull added area: Documentation and removed DNM This PR should not be merged (Do Not Merge) labels Mar 5, 2021
@carlescufi carlescufi requested a review from pabigot March 8, 2021 11:16
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an opinion on whether this is adequate for Zephyr's needs, but it's a reasonable first step.

https://github.com/pabigot/zephyr/commits/pr/32972 has two fixup commits that complete the terminology set and the proposed effect on the api in #18970 (comment)

@gmarull gmarull marked this pull request as ready for review March 25, 2021 09:25
@gmarull gmarull added the dev-review To be discussed in dev-review meeting label Mar 29, 2021
@galak
Copy link
Collaborator

galak commented Apr 1, 2021

dev-review (Apr 1):

  • Add a comma or some visual separator for the list
  • Add something about function properties being a WIP
  • plan to merge this to enable being able to update a subsystem with function properties

@galak galak removed the dev-review To be discussed in dev-review meeting label Apr 1, 2021
Add aliases for setting function properties. A function may have 0, one
or more function properties. Example usage:

@funcprops \isr_ok, \async

These aliases will translate to API Terminology references when Doxygen
is rendered on Sphinx. On the Doxygen side they will just translate to
plain text.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Replace old notes marking ISR safe functions with the recently
introduced funcprops.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
@gmarull
Copy link
Member Author

gmarull commented Apr 5, 2021

Updated PR:

  • Function properties title moved to Function properties (list may not be complete). Breathe doesn't seem to render \n, so it is not easy/possible to render this information afterwards. In my opinion adding this information is misleading. Current docs may also suffer this problem.
  • Regarding comma, it is actually possible to write @funcprops \prop0, \prop1, ..., \propN, so I haven't changed anything.

image
image

@gmarull
Copy link
Member Author

gmarull commented Apr 16, 2021

@nashif @carlescufi ping

Copy link
Member

@dcpleung dcpleung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is possible to have a brief "pop-up" description when you hover over the properties.

@gmarull
Copy link
Member Author

gmarull commented Apr 19, 2021

I wonder if it is possible to have a brief "pop-up" description when you hover over the properties.

Afaik this is not trivial to do, would require a Sphinx plugin at least.

@nashif nashif merged commit 9de14e8 into zephyrproject-rtos:master Apr 28, 2021
@gmarull gmarull deleted the feature/add-funcprops branch April 28, 2021 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document where APIs can be called from using doxygen
6 participants